issue #628 save_user_coords - #629
Conversation
88a83ba to
2d3306a
Compare
091137a to
ad8b421
Compare
cccc1cd to
4a5f2f4
Compare
4a5f2f4 to
b4cb1a9
Compare
e8d3b16 to
0462c38
Compare
0462c38 to
e24323f
Compare
|
If you want a feedback from c:geo side, please open an issue there. |
e24323f to
5194518
Compare
Use a simple '' instead of Db::escape_string("") for the empty
description value in the INSERT statement.
b8d1c60 to
60a163a
Compare
| else # oc.pl branch | ||
| { | ||
| $rs = Db::query(" | ||
| select max(id) as id | ||
| from cache_mod_cords | ||
| where | ||
| cache_id = '".Db::escape_string($cache_id)."' | ||
| and user_id = '".Db::escape_string($user_id)."' | ||
| "); | ||
| $id = null; | ||
| if($row = Db::fetch_assoc($rs)) { | ||
| $id = $row['id']; | ||
| } | ||
| if ($id == null) { | ||
| Db::query(" | ||
| insert into cache_mod_cords ( | ||
| cache_id, user_id, latitude, longitude | ||
| ) values ( | ||
| '".Db::escape_string($cache_id)."', | ||
| '".Db::escape_string($user_id)."', | ||
| '".Db::escape_string($latitude)."', | ||
| '".Db::escape_string($longitude)."' | ||
| ) | ||
| "); | ||
| } else { | ||
| Db::query(" | ||
| update cache_mod_cords | ||
| set latitude = '".Db::escape_string($latitude)."', | ||
| longitude = '".Db::escape_string($longitude)."' | ||
| where | ||
| id = '".Db::escape_string($id)."' | ||
| "); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
OCPL cache_mod_cords table does not contain an id column.
create table cache_mod_cords
(
cache_id int not null,
user_id int not null,
date timestamp default current_timestamp() not null,
longitude double not null,
latitude double not null,
primary key (cache_id, user_id)
)
it should look something like this:
else # oc.pl branch
{
Db::query("
INSERT INTO cache_mod_cords (
cache_id,
user_id,
latitude,
longitude,
date
) VALUES (
'".Db::escape_string($cache_id)."',
'".Db::escape_string($user_id)."',
'".Db::escape_string($latitude)."',
'".Db::escape_string($longitude)."',
NOW()
)
ON DUPLICATE KEY UPDATE
latitude = VALUES(latitude),
longitude = VALUES(longitude),
date = NOW()
");
}
Additionally, in OCPL only some cache types support user coordinates:
https://github.com/opencaching/opencaching-pl/blob/30ae3a4681c6840fe5aad454ccdb50fdb1340603/src/Controllers/ViewCacheController.php#L646-L648
There was a problem hiding this comment.
I have commited your suggested else path.
With respect go the cache types that support user coordinates: in OCDE all do. I don't know how to access ocpl source code so pls send me the list of cache types that do support it then I will add the filtering.
cache_mod_cords has a composite PK (cache_id, user_id) with no id column, so the previous select max(id) would fail at runtime.
stefopl
left a comment
There was a problem hiding this comment.
The documentation should also be updated to include these changes.
| 'services/caches/geocache', | ||
| new OkapiInternalRequest($request->consumer, $request->token, array( | ||
| 'cache_code' => $cache_code, | ||
| 'fields' => 'internal_id' |
There was a problem hiding this comment.
| 'fields' => 'internal_id' | |
| 'fields' => 'internal_id|type' |
| )) | ||
| ); | ||
| $cache_id = $geocache['internal_id']; | ||
|
|
There was a problem hiding this comment.
| self::validate_cache_type($geocache['type']); | |
| ); | ||
| return Okapi::formatted_response($request, $result); | ||
| } | ||
|
|
There was a problem hiding this comment.
| private static function validate_cache_type($cache_type) | |
| { | |
| if (Settings::get('OC_BRANCH') != 'oc.pl') { | |
| return; | |
| } | |
| $allowed_types = array( | |
| 'Other', | |
| 'Quiz', | |
| 'Multi', | |
| ); | |
| if (!in_array($cache_type, $allowed_types, true)) { | |
| throw new InvalidParam( | |
| 'cache_code', | |
| "User coordinates are not supported for cache type '$cache_type'." | |
| ); | |
| } | |
| } |
OCPL only supports user coordinates for Multi, Quiz, and Other cache types (per ViewCacheController). Fetch the type field alongside internal_id and reject unsupported types with HTTP 400. Also document the OCPL restriction in docs.xml.
|
Thanks @stefopl — all suggestions applied in d27f91d:
(Personally I think OCPL should support user coordinates for all cache types, but that's a separate discussion — scoping it to the platform constraint for now.) |
mlekorlz
left a comment
There was a problem hiding this comment.
Tested end to end on both branches with a real Level 3 OAuth token:
- OCPL: local install from the devel dump, PHP 8.1
- OCDE: local
oc-server3(development) on ddev, PHP 8.2
Both at 3aab5717 with this branch applied.
The cache_mod_cords fix from the last round is right. The table has a composite primary key and
no id column, so INSERT ... ON DUPLICATE KEY UPDATE is the correct shape:
CREATE TABLE `cache_mod_cords` (
`cache_id` int(11) NOT NULL,
`user_id` int(11) NOT NULL,
`date` timestamp NOT NULL DEFAULT current_timestamp(),
`longitude` double NOT NULL,
`latitude` double NOT NULL,
PRIMARY KEY (`cache_id`,`user_id`),
KEY `user_id` (`user_id`)
)
| what I ran | OCPL | OCDE |
|---|---|---|
| first save on an allowed cache type | {"success":true}, one row written |
{"success":true}, one row written |
| second save, different coordinates | still one row, coords and date updated |
still one row, updated in place (id unchanged, date_created kept, last_modified bumped) |
| save on a Traditional cache | HTTP 400, "User coordinates are not supported for cache type 'Traditional'" |
accepted, as intended since the type gate is oc.pl only |
read back via services/caches/geocache alt_wpts |
returned as a user-coords waypoint with the saved coordinates |
same, alongside the cache's existing parking waypoint |
Two things I checked specifically because they are branch-dependent:
coordinates on OCDE has date_created and last_modified as NOT NULL with no default, and the
insert here does not set them. That is fine: coordinatesBeforeInsert / coordinatesAfterInsert
fill them, so the insert succeeds even under STRICT_TRANS_TABLES, which is what the ddev install
runs.
The type = 2 constant matches the read side. services/caches/geocaches/WebService.php:1391-1395
already selects from coordinates ... and type = 2 for oc.de, and the round trip above confirms
the reader picks up exactly what this method writes.
No concerns from my side.
This is about adding a new service endpoint. The platforms (opencaching.xx) provide the capabilities to maintain a personal cache notes field for each cache. In addition with this there is also a capability to store user provided coordinates for instance the coordinates that are the result of solving a puzzle.
The OKAPI provides a service to retrieve the personal caches notes and also the user_coords. It also provides a service to update the personal cache notes but not the user coords.
I have implemented a new service to fill this gap. First I thought about extending the already existing service
:: services/caches/save_personal_notes
however, to make sure that this doesn't create any adverse side effects for existing client applications I decided to implement a new service:
:: services/caches/save_user_coords.